Skip to content

Harden db defined type #1484

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Aug 23, 2022
Merged

Harden db defined type #1484

merged 4 commits into from
Aug 23, 2022

Conversation

chelnak
Copy link
Contributor

@chelnak chelnak commented Aug 16, 2022

Prior to this commit there was a possibility that malformed strings could be passed in to the resource.
This could lead to unsafe executions on a remote system.

The parameters that are susceptible are dbname, sql and import_cat_cmd.

In addition, commands were not properly broken out in to arrays of arguments when passed to the exec resource.

This commit fixes the above by adding validation to parameters (where possible) to ensure that the given values conform to expectation.

dbname is validated with a regular expression that ensures that it matches expectations set by mysql.

sql now only accepts values as an Array[String].
This can be one or more value.
Values are also validated with a regular expression that ensures that the given paths are valid.

import_cat_cmd has been removed in favour of a boolean parameter called use_zcat.
Setting this parameter to true will set the value of the private import_cat_cmd to zcat.

This commit also contains some minor refactoring.

@chelnak chelnak self-assigned this Aug 16, 2022
@chelnak chelnak requested a review from a team as a code owner August 16, 2022 14:10
@puppet-community-rangefinder
Copy link

mysql::db is a type

Breaking changes to this file WILL impact these 67 modules (exact match):
Breaking changes to this file MAY impact these 16 modules (near match):

This module is declared in 140 of 579 indexed public Puppetfiles.

These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

@chelnak chelnak changed the title Maint harden db defined type Harden db defined type Aug 16, 2022
LivingInSyn
LivingInSyn previously approved these changes Aug 17, 2022
View reviewed changes
@chelnak chelnak force-pushed the maint-harden_db_defined_type branch from 25efcd2 to 4718bba Compare August 17, 2022 16:07
@chelnak chelnak requested a review from a team August 17, 2022 16:09
@chelnak chelnak force-pushed the maint-harden_db_defined_type branch from 4718bba to ca03fb2 Compare August 17, 2022 21:35
@chelnak chelnak requested a review from LivingInSyn August 17, 2022 21:42
@chelnak chelnak requested review from a team and binford2k August 19, 2022 11:39
LivingInSyn
LivingInSyn previously approved these changes Aug 19, 2022
View reviewed changes
@binford2k
Copy link
Contributor

I don't think that use_zcat is sufficient. For example, one might want to use bzcat. What if we kept import_cat_command but specified its data type as Enum['cat', 'zcat', 'bzcat']? Are there other commands that make sense? I don't think we need to support bat and all the other fancy pants alternatives, just posix.

One could argue for the ability to specify a full path, like /usr/local/bin/cat but I think that's a bad idea and opens us up to undefined behavior.

@LivingInSyn
Copy link

I don't think that use_zcat is sufficient. For example, one might want to use bzcat. What if we kept import_cat_command but specified its data type as Enum['cat', 'zcat', 'bzcat']? Are there other commands that make sense? I don't think we need to support bat and all the other fancy pants alternatives, just posix.

One could argue for the ability to specify a full path, like /usr/local/bin/cat but I think that's a bad idea and opens us up to undefined behavior.

I think we should have it as an enum with the most common options in there. We can always accept PRs for new ones

@chelnak
Copy link
Contributor Author

chelnak commented Aug 19, 2022

Enum sounds sensible to me... and actually removes a breaking element of the change.

chelnak added 3 commits August 22, 2022 09:58
@chelnak
Harden db defined type
547483f 
Prior to this commit there was a possibility that malformed strings
could be passed in to the resource. This could lead to unsafe executions on
a remote system.

The parameters that are susceptible are `dbname`, `sql` and `import_cat_cmd`.

In addition, commands were not properly broken out in to arrays of
arguments when passed to the exec resource.

This commit fixes the above by adding validation to parameters (where possible)
to ensure that the given values conform to expectation.

`dbname` is validated with a regular expression that ensures that it matches
expectations set by mysql.

`sql` now only accepts values as an `Array[String]`. This can be one or more value.
Values are also validated with a regular expression that ensures that the given
paths are valid.

`import_cat_cmd` has been removed in favour of a boolean parameter called `use_zcat`.
Setting this parameter to `true` will set the value of the private `import_cat_cmd`
to `zcat`.

This commit also contains some minor refactoring.
@chelnak
Add spec tests
211de4f 
This commit adds spec tests for the the changes made in the previous
commit.
@chelnak
Fixes for acceptance tests
603287f 
This commit fixes some small issues where tests were failing because
of a type mismatch.
@chelnak chelnak force-pushed the maint-harden_db_defined_type branch from ca03fb2 to 603287f Compare August 22, 2022 10:01
@chelnak chelnak requested a review from a team August 22, 2022 10:05
jelinwils
jelinwils previously approved these changes Aug 22, 2022
View reviewed changes
binford2k
binford2k previously requested changes Aug 22, 2022
View reviewed changes
Copy link
Contributor

@binford2k binford2k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do think that we need bzcat in the command options, otherwise this looks good

@chelnak @binford2k
Update manifests/db.pp
cd2e485 
Co-authored-by: Ben Ford <ben.ford@puppetlabs.com>
GSPatton
GSPatton approved these changes Aug 23, 2022
View reviewed changes
Copy link
Contributor

@GSPatton GSPatton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all LGTM

@chelnak chelnak dismissed binford2k’s stale review August 23, 2022 08:57

Implemented change

@GSPatton GSPatton merged commit f83792b into main Aug 23, 2022
@GSPatton GSPatton deleted the maint-harden_db_defined_type branch August 23, 2022 08:58
@Rathios
Copy link

Rathios commented Sep 10, 2022

This has already been merged, but .. you can easily support both string and array in the sql parameter by doing flatten([$sql]). Would have saved some needless breakage.

Error while evaluating a Resource Statement, Mysql::Db[powerdns]: parameter 'sql' expects a value of type Undef or Array, got String (file: /etc/puppetlabs/code/environments/dev/modules/powerdns/manifests/backends/mysql.pp, line: 70) on node x

@apoleon
Copy link

apoleon commented Oct 30, 2022

Hello,

is this the fix for CVE-2022-3276 or are #1487 #1486 1485 also needed to resolve it ?

@chelnak
Copy link
Contributor Author

chelnak commented Oct 30, 2022 •
edited
Loading

You will need all of them. We recommend consuming the latest major release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GSPatton GSPatton GSPatton approved these changes

@binford2k binford2k binford2k left review comments

@LivingInSyn LivingInSyn LivingInSyn left review comments

@jelinwils jelinwils jelinwils left review comments

Assignees

@chelnak chelnak

Labels
backwards-incompatible bugfix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@chelnak @binford2k @LivingInSyn @Rathios @apoleon @GSPatton @jelinwils